Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow extending Form/Control/Upload class by using different JS plugin #2200

Conversation

harish2704
Copy link

This PR allow us to create custom upload widgets by extending Upload or UploadImage class.

( Context: I tried to create CroppedUploadImage widget class which will allow user to crop the image before upload )

Copy link
Member

@mvorisek mvorisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it is not good practice to support different jQuery method name as it will address an usecase only when the accepted args are the same.

As you already can/registered jQuery method in your application, instead of registering a new one, you should override the atk4 jQuery atkFileUpload method to call your extended function possibly if some other conditions are met.

@mvorisek mvorisek closed this Apr 7, 2024
@harish2704
Copy link
Author

harish2704 commented Apr 7, 2024

@mvorisek : Thank for your quick response.

Regarding your comments,

In general, it is not good practice to support different jQuery method name as it will address an usecase only when the accepted args are the same.

Yes, I do agree that it is not a perfect choice or practice, but in this way at-least we get some extensibility through which at-least some of the usecases can be fulfilled .
The current situation worse than this .

As you already can/registered jQuery method in your application, instead of registering a new one, you should override the atk4 jQuery atkFileUpload method to call your extended function possibly if some other conditions are met.

IMHO, it is more hackish method than the suggested one.
First of all, arguments passed to this plugin call are fixed. More hacks will be required to create "matching conditions"
Second, because of the previous issue, the default behavior of existing upload or uploadImage may break if the customization is in effect
Third, I think some JS level monkey patching will be required to override the registered JS plugin ( I am not sure about it )

In short, The suggested change at-least opens a way for others to extend functionality by writing a plugin following same public interface ( function arguments ) even though it is not a perfect solution

@mvorisek
Copy link
Member

mvorisek commented Apr 7, 2024

I agree, but we need something conceptually working for every component, otherwise we will hack more and more.

If you do not want to solve this in JS now, you can override Upload::js() method to return a custom JS chain and replace (first chain call) "atkFileUpload" there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants